Skip to content

Introduce OAuth2AuthorizationRequest.attributes #6508

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Introduce OAuth2AuthorizationRequest.attributes #6508

wants to merge 2 commits into from

Conversation

jgrandja
Copy link
Contributor

@jgrandja jgrandja commented Feb 6, 2019

Fixes #5940

@jgrandja jgrandja requested a review from rwinch February 6, 2019 10:16
rwinch
rwinch requested changes Feb 6, 2019
View reviewed changes
...ringframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java
@@ -105,7 +105,7 @@ public void resolveWhenAuthorizationRequestWithValidClientThenResolves() {
.isEqualTo("http://localhost/login/oauth2/code/" + clientRegistration.getRegistrationId());
assertThat(authorizationRequest.getScopes()).isEqualTo(clientRegistration.getScopes());
assertThat(authorizationRequest.getState()).isNotNull();
assertThat(authorizationRequest.getAdditionalParameters())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is to fix gh-5940 we should assert that the additional parameters do not contain the registration id (just because it is in the attributes -- the next assertion -- doesn't mean it isn't also in the parameters)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Applied and pushed.

rwinch
rwinch approved these changes Feb 7, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It looks good to me now. Can you please squash and merge when you are ready?

NOTE: It looks like travis had an error (couldn't find the .travis.yml file). Obviously this isn't an issue with this PR, but we don't know if it passes either. You might double check that it builds locally or kick off another build.

@jgrandja jgrandja added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Feb 7, 2019
@jgrandja jgrandja added this to the 5.2.0.M2 milestone Feb 7, 2019
@jgrandja
Copy link
Contributor Author

jgrandja commented Feb 7, 2019

Thanks @rwinch. I tried building locally and it passed.

@jgrandja
Copy link
Contributor Author

jgrandja commented Feb 7, 2019

Merged via 594a169

@jgrandja jgrandja closed this Feb 7, 2019
@jgrandja jgrandja deleted the gh-5940-authz-req-attrs branch February 7, 2019 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch approved these changes

Assignees
No one assigned
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.M2
Development

Successfully merging this pull request may close these issues.
2 participants
@jgrandja @rwinch